-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java: model Spring web.method.support #6595
base: main
Are you sure you want to change the base?
Conversation
acfcd5c
to
6682d93
Compare
row = | ||
[ | ||
// for review: arguably this shouldn't be modeled as the implementations of resolveArgument that I've seen are effectively sanitized | ||
"org.springframework.web.method.support;HandlerMethodArgumentResolver;true;resolveArgument;;;Argument[2];ReturnValue;taint", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give an example what sanitisation you've seen? If the user provides a form value or whatever is going to be associated with the given method parameter, will it be rendered safe for pasting into HTML output? Would it still work as SQL injection for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the two cases I saw in Spring data, the user-input strings were parsed into a Sort
object, and page size and page number, neither of which I think can carry meaningful taint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sure, but presumably this would propagate taint when the argument type isn't restrictive like that? If so I'd say we should regard it as a taint propagator in general and sanitize depending on the particular usage (e.g. integers are already blanket sanitized by the XSS query)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was my thinking. I'll leave this in then.
"org.springframework.web.method.support;UriComponentsContributor;true;contributeMethodArgument;;;Argument[1];Argument[2];taint", | ||
"org.springframework.web.method.support;UriComponentsContributor;true;contributeMethodArgument;;;Argument[1];Argument[3];taint", | ||
// InvocableHandlerMethod is not modeled as it is difficult to model method-like classes with CSV | ||
// This is a very broad definition of data flow; there is a method `setRedirectModelScenario(boolean)` which is used to determine which of the `Default` and `Redirect` models are returned by `getModel`, and the methods that deal with attributes below are convenience methods for `.getMethod().*`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.getModel().*?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. addAttribute
is a shortcut for getModel().addAttribute
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebMethod.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Chris Smowton <[email protected]>
No description provided.